Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuration of replay thread pools from CLI #236

Merged
merged 14 commits into from
Mar 20, 2024

Conversation

steviez
Copy link

@steviez steviez commented Mar 14, 2024

Problem

We want to be able to control threadpool sizes on the CLI - see #105 for more context

Summary of Changes

  • Introduce two new arguments to the CLI
    • One to control the number of threads for replaying multiple forks in parallel (this is a replacement for --replay-slots-concurrently which was a boolean with hard-coded number of threads if true)
    • One to control the size of the thread-pool used to execute transactions
  • Deprecate the pre-existing --replay-slots-concurrently argument

I did a sanity check real quick to make sure nothing broke on CLI. Namely,

  • Specify neither of the two new args and ensure default line up with existing behavior
  • Specify custom values for each and observe those taking effect
  • Specify --replay-slots-concurrently and --replay-forks-threads and observed error for conflict as expected

Lastly, there is another somewhat related pool that we may wish to control, the one used to replay local ledger before ReplayStage is created. While that is related, I think we can push to another PR, especially since we'll want an accompanying argument added to ledger-tool as well.

@steviez steviez marked this pull request as ready for review March 14, 2024 06:18
core/src/replay_stage.rs Show resolved Hide resolved
core/src/tvu.rs Show resolved Hide resolved
core/src/validator.rs Outdated Show resolved Hide resolved
validator/src/cli/thread_args.rs Show resolved Hide resolved
validator/src/cli/thread_args.rs Show resolved Hide resolved
validator/src/cli/thread_args.rs Outdated Show resolved Hide resolved
@steviez steviez requested review from t-nelson and bw-solana March 14, 2024 06:41
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 27.77778% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (67c3bff) to head (2b50424).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #236     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         837      838      +1     
  Lines      226874   226913     +39     
=========================================
- Hits       185870   185862      -8     
- Misses      41004    41051     +47     

Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM. Left a couple of minor comments

validator/src/cli/thread_args.rs Outdated Show resolved Hide resolved
validator/src/cli/thread_args.rs Outdated Show resolved Hide resolved
core/src/validator.rs Show resolved Hide resolved
core/src/tvu.rs Show resolved Hide resolved
validator/src/main.rs Outdated Show resolved Hide resolved
validator/src/main.rs Outdated Show resolved Hide resolved
@steviez steviez force-pushed the cli_replay_threads branch from 3887be2 to f117926 Compare March 18, 2024 18:47
@steviez steviez requested review from t-nelson and bw-solana March 18, 2024 20:17
@steviez steviez force-pushed the cli_replay_threads branch from f117926 to 1f5bac4 Compare March 19, 2024 05:16
@steviez
Copy link
Author

steviez commented Mar 19, 2024

Force pushed to rebase on tip of master to resolve a conflict

bw-solana
bw-solana previously approved these changes Mar 19, 2024
Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

steviez added 3 commits March 19, 2024 14:17
The trait will reduce redundant code and further enforce these args all
being uniform with each other, both in the source code and on the CLI at
runtime
@steviez steviez requested a review from bw-solana March 19, 2024 20:05
Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@steviez steviez merged commit 4a67cd4 into anza-xyz:master Mar 20, 2024
35 checks passed
@steviez steviez deleted the cli_replay_threads branch March 20, 2024 20:07
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
Bubble up the constants to the CLI that control the sizes of the
following two thread pools:
- The thread pool used to replay multiple forks in parallel
- The thread pool used to execute transactions in parallel
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants